[FEA] Report all unsupported operations for a query in cudf.polars #16960

Matt711 · 2024-10-01T02:05:29Z

Description

Closes #16690. The purpose of this PR is to list all of the unique operations that are unsupported by cudf.polars when running a query.

Question: How to traverse the tree to report the error nodes? Should this be done upstream in Polars?
Instead of traversing the query afterwards, we should probably catch each unsupported feature as we translate the IR.

Checklist

I am familiar with the Contributing Guidelines.
New or existing tests cover these changes.
The documentation is up to date with these changes.

python/cudf_polars/cudf_polars/dsl/ir.py

python/cudf_polars/cudf_polars/utils/other.py

python/cudf_polars/cudf_polars/dsl/translate.py

python/cudf_polars/cudf_polars/callback.py

python/cudf_polars/cudf_polars/dsl/translate.py

wence-

Thanks, this is looking really nice. Some smaller suggestions and a few small logic fixes

python/cudf_polars/cudf_polars/dsl/translate.py

python/cudf_polars/cudf_polars/callback.py

python/cudf_polars/tests/test_translate_in_debug_mode.py

python/cudf_polars/cudf_polars/callback.py

wence-

Tiny suggestions, debug_mode is now gone.

python/cudf_polars/cudf_polars/dsl/translate.py

Matt711

I changed the logic of assert_ir_translation_raises to integrate with the changes made in this PR. I made the changes because now that we're returning ErrorNodes and ErrorExprs instead of raising exceptions during translation, assert_ir_translation_raises(q, NotImplementedError) fails in a lot of places.

To solve this, I checked that the exception(s) being asserted in q.collect(...) are inside Translation.errors and treated any other exceptions raised during translation as cases where assert_ir_translation_raises fails. I this required me to hard-code a few cases where translation could fail.

Are there other cases I missed where translation could fail? WDYT of the changes @wence-?

python/cudf_polars/tests/test_config.py

python/cudf_polars/cudf_polars/testing/asserts.py

python/cudf_polars/cudf_polars/dsl/translate.py

python/cudf_polars/cudf_polars/testing/asserts.py

wence- · 2024-10-30T17:19:34Z

@Matt711 This stalled a bit, I think it's a good one to get into 24.12, do you need some help with some bits?

Matt711 · 2024-10-31T00:39:24Z

@Matt711 This stalled a bit, I think it's a good one to get into 24.12, do you need some help with some bits?

Hey @wence-, that should be doable. The last thing I needed to do with this PR is get test coverage to 100%. I'll address merge conflicts tomorrow, and try to do that too. I'll check in offline if I need help.

copy-pr-bot · 2024-10-31T15:02:51Z

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

wence-

Looks good @Matt711, I had one suggestion that may help your segfault issue

python/cudf_polars/cudf_polars/callback.py

python/cudf_polars/cudf_polars/dsl/translate.py

wence- · 2024-11-05T16:59:28Z

python/cudf_polars/cudf_polars/dsl/translate.py

+                schema = {k: dtypes.from_polars(v) for k, v in polars_schema.items()}
+            except Exception as e:
+                self.errors.append(e)
+                return ir.ErrorNode({}, str(e))


Hmm, a tricky one, we want to put schema here, but we didn't manage to make it.

python/cudf_polars/cudf_polars/dsl/translate.py

python/cudf_polars/cudf_polars/testing/asserts.py

wence- · 2024-11-05T17:12:11Z

python/cudf_polars/cudf_polars/testing/plugin.py

@@ -45,6 +45,7 @@ def pytest_configure(config: pytest.Config) -> None:


 EXPECTED_FAILURES: Mapping[str, str] = {
+    "tests/unit/dataframe/test_df.py::test_extension": "AssertionError",


No idea, sorry

…ed-ops

Matt711 · 2024-11-07T04:23:20Z

/ok to test

…om:Matt711/cudf into fea/cudf-polars/report-all-unsupported-ops

Matt711 · 2024-11-07T18:16:07Z

/ok to test

python/cudf_polars/tests/test_mapfunction.py

Matt711 · 2024-11-07T19:00:10Z

/ok to test

Matt711 · 2024-11-08T15:55:11Z

/ok to test

vyasr

The changeset looks large, but am I correct that ultimately all that happened is that free functions translate_ir and translate_expr were made methods of the new Translator class so that this class could maintain a list of errors raised in any of those calls during a single traversal? If so, then I've grokked this PR and generally everything looks fine to me and I don't need to review again.

python/cudf_polars/cudf_polars/testing/asserts.py

vyasr · 2024-11-08T18:51:59Z

python/cudf_polars/cudf_polars/testing/plugin.py

@@ -45,6 +45,7 @@ def pytest_configure(config: pytest.Config) -> None:


 EXPECTED_FAILURES: Mapping[str, str] = {
+    "tests/unit/dataframe/test_df.py::test_extension": "AssertionError",


Is this still failing? Not sure what the current state is.

python/cudf_polars/cudf_polars/utils/dtypes.py

wence- · 2024-11-08T20:32:26Z

The changeset looks large, but am I correct that ultimately all that happened is that free functions translate_ir and translate_expr were made methods of the new Translator class so that this class could maintain a list of errors raised in any of those calls during a single traversal? If so, then I've grokked this PR and generally everything looks fine to me and I don't need to review again.

Basically yes

Matt711 · 2024-11-08T21:12:28Z

/ok to test

…ed-ops

Matt711 · 2024-11-09T00:34:54Z

/ok to test

ci/run_cudf_polars_polars_tests.sh

Matt711 · 2024-11-11T18:57:07Z

/ok to test

wence-

Thanks @Matt711, nice work!

wence- · 2024-11-12T12:12:00Z

/merge

Matt711 added feature request New feature or request 5 - DO NOT MERGE Hold off on merging; see PR for details non-breaking Non-breaking change labels Oct 1, 2024

Matt711 self-assigned this Oct 1, 2024

github-actions bot added Python Affects Python cuDF API. cudf.polars Issues specific to cudf.polars labels Oct 1, 2024

wence- reviewed Oct 1, 2024

View reviewed changes

python/cudf_polars/cudf_polars/dsl/ir.py Outdated Show resolved Hide resolved

python/cudf_polars/cudf_polars/utils/other.py Outdated Show resolved Hide resolved

python/cudf_polars/cudf_polars/dsl/translate.py Outdated Show resolved Hide resolved

Matt711 commented Oct 2, 2024

View reviewed changes

python/cudf_polars/cudf_polars/callback.py Outdated Show resolved Hide resolved

wence- requested changes Oct 8, 2024

View reviewed changes

python/cudf_polars/cudf_polars/callback.py Outdated Show resolved Hide resolved

python/cudf_polars/cudf_polars/dsl/translate.py Outdated Show resolved Hide resolved

Matt711 force-pushed the fea/cudf-polars/report-all-unsupported-ops branch from 0310f26 to 3175a7e Compare October 9, 2024 03:46

wence- reviewed Oct 9, 2024

View reviewed changes

python/cudf_polars/cudf_polars/dsl/translate.py Outdated Show resolved Hide resolved

Matt711 force-pushed the fea/cudf-polars/report-all-unsupported-ops branch from 3175a7e to 054b271 Compare October 9, 2024 15:28

wence- reviewed Oct 9, 2024

View reviewed changes

Matt711 commented Oct 9, 2024

View reviewed changes

python/cudf_polars/tests/test_translate_in_debug_mode.py Outdated Show resolved Hide resolved

wence- reviewed Oct 10, 2024

View reviewed changes

python/cudf_polars/cudf_polars/callback.py Outdated Show resolved Hide resolved

wence- reviewed Oct 10, 2024

View reviewed changes

python/cudf_polars/cudf_polars/dsl/translate.py Outdated Show resolved Hide resolved

python/cudf_polars/cudf_polars/dsl/translate.py Outdated Show resolved Hide resolved

python/cudf_polars/cudf_polars/dsl/translate.py Outdated Show resolved Hide resolved

Matt711 mentioned this pull request Oct 10, 2024

[FEA] Display operations unsupported by the GPU in a user-friendly way #17051

Open

Matt711 commented Oct 10, 2024

View reviewed changes

wence- reviewed Oct 11, 2024

View reviewed changes

python/cudf_polars/cudf_polars/testing/asserts.py Outdated Show resolved Hide resolved

Matt711 added 6 commits October 31, 2024 07:56

address review

b74bfa9

add a test

a0f5734

add a test

5a1e85f

address review

192e769

address review and fix failing tests

a67bb8b

get code cooverage to pass

9551c1f

Matt711 force-pushed the fea/cudf-polars/report-all-unsupported-ops branch from ff7f2e1 to 9551c1f Compare October 31, 2024 15:02

more merge conflicts

3b770f9

wence- reviewed Nov 5, 2024

View reviewed changes

Matt711 and others added 4 commits November 6, 2024 10:20

merge conflict

b5d21f4

address review

02697e8

address review

349309a

Merge branch 'branch-24.12' into fea/cudf-polars/report-all-unsupport…

588b21f

…ed-ops

Matt711 added 2 commits November 7, 2024 10:13

Address merge conflict and review

a079355

Merge branch 'fea/cudf-polars/report-all-unsupported-ops' of github.c…

2d3b36f

…om:Matt711/cudf into fea/cudf-polars/report-all-unsupported-ops

remove test

1207ba3

Matt711 commented Nov 7, 2024

View reviewed changes

python/cudf_polars/tests/test_mapfunction.py Show resolved Hide resolved

fix code cov

40579f5

vyasr approved these changes Nov 8, 2024

View reviewed changes

address review, and remove xfail from polars test

a56d5fc

Matt711 and others added 2 commits November 8, 2024 19:30

Merge branch 'branch-24.12' into fea/cudf-polars/report-all-unsupport…

0403bc9

…ed-ops

verbose

ba98f03

Matt711 requested a review from a team as a code owner November 9, 2024 00:34

Matt711 commented Nov 9, 2024

View reviewed changes

ci/run_cudf_polars_polars_tests.sh Outdated Show resolved Hide resolved

Matt711 added 2 commits November 11, 2024 10:50

recreate the exception and append to errors list

b0f8168

remove ci change

b362d64

Matt711 requested a review from wence- November 11, 2024 22:54

wence- approved these changes Nov 12, 2024

View reviewed changes

rapids-bot bot merged commit 043bcbd into rapidsai:branch-24.12 Nov 12, 2024
102 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FEA] Report all unsupported operations for a query in cudf.polars #16960

[FEA] Report all unsupported operations for a query in cudf.polars #16960

Matt711 commented Oct 1, 2024 •

edited

Loading

wence- left a comment

wence- left a comment

Matt711 left a comment

wence- commented Oct 30, 2024

Matt711 commented Oct 31, 2024

copy-pr-bot bot commented Oct 31, 2024

wence- left a comment

wence- Nov 5, 2024

wence- Nov 5, 2024

Matt711 commented Nov 7, 2024

Matt711 commented Nov 7, 2024

Matt711 commented Nov 7, 2024

Matt711 commented Nov 8, 2024

vyasr left a comment

vyasr Nov 8, 2024

wence- commented Nov 8, 2024

Matt711 commented Nov 8, 2024

Matt711 commented Nov 9, 2024

Matt711 commented Nov 11, 2024

wence- left a comment

wence- commented Nov 12, 2024

		@@ -45,6 +45,7 @@ def pytest_configure(config: pytest.Config) -> None:


		EXPECTED_FAILURES: Mapping[str, str] = {
		"tests/unit/dataframe/test_df.py::test_extension": "AssertionError",

[FEA] Report all unsupported operations for a query in cudf.polars #16960

[FEA] Report all unsupported operations for a query in cudf.polars #16960

Conversation

Matt711 commented Oct 1, 2024 • edited Loading

Description

Checklist

wence- left a comment

Choose a reason for hiding this comment

wence- left a comment

Choose a reason for hiding this comment

Matt711 left a comment

Choose a reason for hiding this comment

wence- commented Oct 30, 2024

Matt711 commented Oct 31, 2024

copy-pr-bot bot commented Oct 31, 2024

wence- left a comment

Choose a reason for hiding this comment

wence- Nov 5, 2024

Choose a reason for hiding this comment

wence- Nov 5, 2024

Choose a reason for hiding this comment

Matt711 commented Nov 7, 2024

Matt711 commented Nov 7, 2024

Matt711 commented Nov 7, 2024

Matt711 commented Nov 8, 2024

vyasr left a comment

Choose a reason for hiding this comment

vyasr Nov 8, 2024

Choose a reason for hiding this comment

wence- commented Nov 8, 2024

Matt711 commented Nov 8, 2024

Matt711 commented Nov 9, 2024

Matt711 commented Nov 11, 2024

wence- left a comment

Choose a reason for hiding this comment

wence- commented Nov 12, 2024

Matt711 commented Oct 1, 2024 •

edited

Loading